Allow linker to perform deadcode elimination for program using Cobra #1956

aarzilli · 2023-05-05T11:49:51Z

Fixes #2015

Cobra, in its default configuration, will execute a template to generate help, usage and version outputs. Text/template execution calls MethodByName and MethodByName disables dead code elimination in the Go linker, therefore all programs that make use of cobra will be linked with dead code elimination disabled, even if they end up replacing the default usage, help and version formatters with a custom function and no actual text/template evaluations are ever made at runtime.

Dead code elimination in the linker helps reduce disk space and memory utilization of programs. For example, for the simple example program used by TestDeadcodeElimination 40% of the final executable size is dead code. For a more realistic example, 12% of the size of Delve's executable is deadcode.

This PR changes Cobra so that, in its default configuration, it does not automatically inhibit deadcode elimination by:

changing Cobra's default behavior to emit output for usage and help using simple Go functions instead of template execution
quarantining all calls to template execution into SetUsageTemplate, SetHelpTemplate and SetVersionTemplate so that the linker can statically determine if they are reachable

See https://github.com/aarzilli/whydeadcode to help find why dead-code-elimination may not work as well as it could in your project.

CLAassistant · 2023-05-05T11:49:56Z

All committers have signed the CLA.

github-actions · 2023-05-05T11:50:10Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

aarzilli · 2023-05-05T14:53:00Z

The CLA thing isn't working for some reason.

github-actions · 2023-05-05T20:14:54Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2023-06-12T12:39:11Z

Thanks @aarzilli !
I like the idea of optimizing things.
This will require some thought so I can understand clearly the behaviour change. Thanks for adding a test it should help with this.

aarzilli · 2023-06-12T12:51:37Z

Unless I made a mistake there shouldn't be any behavior changes (as in, observable from the outside). Happy to explain anything about this if needed.

aarzilli · 2023-07-28T07:01:10Z

Ping?

aarzilli · 2023-10-10T09:33:54Z

Ping?

github-actions · 2024-02-10T13:44:09Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-04-10T08:01:12Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-05-10T14:50:30Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-06-13T10:01:15Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-08-10T11:31:30Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2024-10-14T01:58:22Z

@aarzilli I apologize for such a long delay, but I had misunderstood the value of this PR. I now realize that it may help all programs using Cobra, so I'm very interested.

I'm trying to convince myself that that this change actually has an impact.
Would you be able to explain how I can see the difference in a program that has dead code, before and after this PR?

Here is what I tried.

I used the program you have added to the tests in this PR
I added an "Unused()" function that I don't call in the program.
I ran go build -o myprog . (using cobra 1.8.1 without your PR)
I ran go tool nm ./myprog | grep Unused

I would have expected to see the Unused function but I don't.
I used go version go1.22.8 darwin/arm64

Could you clarify what I should expect?

This is the program:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

func Unused() {
	fmt.Println("not called")
}

var rootCmd = &cobra.Command{
	Version: "1.0",
	Use:     "example_program",
	Short:   "example_program - test fixture to check that deadcode elimination is allowed",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("hello world")
	},
	Aliases: []string{"alias1", "alias2"},
	Example: "stringer --help",
}

func main() {
	if err := rootCmd.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "Whoops. There was an error while executing your CLI '%s'", err)
		os.Exit(1)
	}
}

aarzilli · 2024-10-14T13:18:20Z

One easy way to see the impact of this change is to compile your example program with and without this PR:

$ ls -lh
total 8.4M
-rwxr-xr-x. 1 a a 5.2M Oct 14 14:45 example.withoutpr*
-rwxr-xr-x. 1 a a 3.2M Oct 14 14:45 example.withpr*
-rw-r--r--. 1 a a  596 Oct 14 14:44 main.go

This is an extreme example, where the size of the executable is reduced by 38%, but reductions of 10% are realistic.
I used go1.23 but you should get very similar results with go1.22.8.

As to your question, saying that deadcode elimination gets disabled is incorrect: in reality it continues to function but in a reduced capacity. Specifically if reflect.Value.MethodByName or reflect.Value.Method are reachable it can not always prove that exported methods are unreachable.

Your Unused function is not an exported method so, unless it is called by an exported method, it can always be deadcode eliminated.

If you want to see the Unused function make it all the way to the executable you have to do something like this:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

type Astruct struct {
	n int
}

//go:noinline
func Unused() {
	fmt.Println("not called")
}

//go:noinline
func (a *Astruct) Unused() {
	fmt.Println("not called", a.n)
	Unused()
}

//go:noinline
func (a *Astruct) Used() {
	fmt.Println("used", a.n)
}

var rootCmd = &cobra.Command{
	Version: "1.0",
	Use:     "example_program",
	Short:   "example_program - test fixture to check that deadcode elimination is allowed",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("hello world")
	},
	Aliases: []string{"alias1", "alias2"},
	Example: "stringer --help",
}

func main() {
	if err := rootCmd.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "Whoops. There was an error while executing your CLI '%s'", err)
		os.Exit(1)
	}
	var a Astruct
	fmt.Println(a)
	a.Used()
}

All of the noinline directives are needed because small functions like this would be removed by the inliner and the fmt.Println call is needed to make the Astruct type itself reachable (if all calls to the methods of Astruct are static the linker can still prove that Unused is unreachable).

With these changes you get:

$ go build main2.go && go tool nm ./main2 | grep 'Unused'
  589c00 T main.(*Astruct).Unused
  589ba0 T main.Unused
  765570 D runtime.adviseUnused
  4179e0 T runtime.sysUnusedOS

without the PR and:

$ go build main2.go && go tool nm ./main2 | grep 'Unused'
  602564 D runtime.adviseUnused
  4144a0 T runtime.sysUnusedOS

with the PR.

marckhouzam · 2024-10-15T03:28:15Z

Thanks for the explanation @aarzilli, I can now see the benefits.

So, with this PR, programs that don't set new templates (usage, help or version) will be able to do dead code elimination fully.

What about programs that do modify those templates? If they want to get full usage of dead code elimination they should convert their template use to a go function like you have done, IIUC? My next step for this review is to try to override the help/usage/version in that fashion and see if it works as expected.

I believe that using SetHelpFunc() and SetUsageFunc() will allow programs to do this, but I don't think we have a way to do this for the version template. But it is ok, it is still better than not being able to do it at all. Once this is merged, we can discuss adding a SetVersionFunc().

marckhouzam

This looks great.
I still want to do some testing, but I don't expect any big surprises.
Here are some comments for the PR.
The PR also needs to be rebased.

Thanks again for your patience, I think we can get this in soon.

cobra_test.go

marckhouzam · 2024-10-15T03:34:55Z

What do you think about updating the documentation in the three sections starting here https://github.com/spf13/cobra/blob/main/site/content/user_guide.md#defining-your-own-help to explain the side-effect of overriding the template, and therefore that it is recommended to set a function instead?

github-actions · 2024-10-15T08:19:54Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

aarzilli · 2024-10-15T08:24:21Z

Thanks for the explanation @aarzilli, I can now see the benefits.

So, with this PR, programs that don't set new templates (usage, help or version) will be able to do dead code elimination fully.

What about programs that do modify those templates? If they want to get full usage of dead code elimination they should convert their template use to a go function like you have done, IIUC?

Yes, this is correct.

I believe that using SetHelpFunc() and SetUsageFunc() will allow programs to do this, but I don't think we have a way to do this for the version template. But it is ok, it is still better than not being able to do it at all. Once this is merged, we can discuss adding a SetVersionFunc().

It imagine should be trivial.

github-actions · 2024-10-15T08:33:47Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

aarzilli · 2025-01-27T13:42:26Z

The changes look good to me.

artem-anisimov-0x7f · 2025-01-27T14:48:41Z

This is a very good improvement. @marckhouzam, could you please make a new release of cobra?

marckhouzam · 2025-01-27T14:49:14Z

@aarzilli thank you for your patience. This took a very long time but you remained very responsive. Your nice attitude (and great technical expertise) made it possible to get this merged. This should benefit many programs!

P.S. I wouldn't be against adding a section to the Cobra documentation teaching people what they should do to try to get dead-code-elimination working. Although this is more of a general Go concept, it might help people understand it if they have a small section in the Cobra docs. But I leave that up to you, if you still have energy after this looooong review.

marckhouzam · 2025-01-27T14:57:27Z

This is a very good improvement. @marckhouzam, could you please make a new release of cobra?

Nice to see immediate interest 😄 .
I'll work on a release this week.

aarzilli · 2025-01-27T18:03:32Z

Thanks for getting this merged!

sylr · 2025-01-29T16:56:32Z

@jpmcb Could we get a cobra release soon to be able to enjoy this change ?

Follow-up to spf13#1956. This commit allows a program to reset any of the tree templates to their default behaviour, as it was possible to do before the change of spf13#1956. Signed-off-by: Marc Khouzam <[email protected]>

Follow-up to #1956. This commit allows a program to reset any of the tree templates to their default behaviour, as it was possible to do before the change of #1956. Signed-off-by: Marc Khouzam <[email protected]>

artem-anisimov-0x7f · 2025-02-12T16:33:40Z

Is there a chance to have v1.9.0 released soon to make this fix available?

This modules uses `github.com/expr-lang/expr` which, because of the extensive use of the `reflect` package, disables go's compiler dead code elimination which can lead to bigger binaries. This commit adds a `noexprlang` build tag that allows jsm.go users that do not use expression matching to entirely disable the use of the expr module so that they can benefit from go's dead code elimination if they are eligible to outside jsm.go. References: - https://golab.io/talks/getting-the-most-out-of-dead-code-elimination - https://github.com/aarzilli/whydeadcode - spf13/cobra#1956 Signed-off-by: Sylvain Rabot <[email protected]>

This module uses `github.com/expr-lang/expr` which, because of the extensive use of the `reflect` package, disables go's compiler dead code elimination which can lead to bigger binaries. This commit adds a `noexprlang` build tag that allows jsm.go users that do not use expression matching to entirely disable the use of the expr module so that they can benefit from go's dead code elimination if they are eligible to outside jsm.go. References: - https://golab.io/talks/getting-the-most-out-of-dead-code-elimination - https://github.com/aarzilli/whydeadcode - spf13/cobra#1956 Signed-off-by: Sylvain Rabot <[email protected]>

marckhouzam · 2025-02-15T18:48:26Z

🎉 https://github.com/spf13/cobra/releases/tag/v1.9.0
Thanks @aarzilli. The dead code elimination improvement will be valuable to many

When reflect.MethodByName is used the linker can not fully perform deadcode elimination. This commit updates cobra and rewrites the suitableMethods part of service/rpccommon so that reflect.MethodByName is not used and the linker can fully execute deadcode elimination. The executable size on go1.24.0 on linux is reduced from 25468606 bytes to 22453382 bytes or a reduction of approximately 12%. See also: spf13/cobra#1956 https://github.com/aarzilli/whydeadcode

When templating is used, the linker cannot know which functions are called and which one are not; this is because tamplating use calls to "MethodByName()" which can be used to call any function (similar to reflection). With the release of Cobra 1.9.1, templates are no longer used by default in Cobra. By also not using templates in the tanzu-plugin-runtime it now gives an opportunity for plugins to try to avoid templates to allow dead-code elimination to work, if they so choose. Ref: spf13/cobra#1956 Signed-off-by: Marc Khouzam <[email protected]>

When templating is used, the linker cannot know which functions are called and which one are not; this is because templating use calls to "MethodByName()" which can be used to call any function (similar to reflection). With the release of Cobra 1.9.1, templates are no longer used by default in Cobra. By also not using templates in the tanzu-plugin-runtime it now gives an opportunity for plugins to try to avoid templates to allow dead-code elimination to work, if they so choose. Ref: spf13/cobra#1956 Signed-off-by: Marc Khouzam <[email protected]>

When templating is used, the linker cannot know which functions are called and which ones are not; this is because templating uses calls to "MethodByName()" which can be used to call any function (similar to reflection). With the release of Cobra 1.9.1, templates are no longer used by default in Cobra. By also not using templates in the tanzu-plugin-runtime it now gives an opportunity for plugins to try to avoid templates to allow dead-code elimination to work, if they so choose. Ref: spf13/cobra#1956 Signed-off-by: Marc Khouzam <[email protected]>

…#228) When templating is used, the linker cannot know which functions are called and which ones are not; this is because templating uses calls to "MethodByName()" which can be used to call any function (similar to reflection). With the release of Cobra 1.9.1, templates are no longer used by default in Cobra. By also not using templates in the tanzu-plugin-runtime it now gives an opportunity for plugins to try to avoid templates to allow dead-code elimination to work, if they so choose. Ref: spf13/cobra#1956 Signed-off-by: Marc Khouzam <[email protected]>

aarzilli force-pushed the dereflect branch from 99d7f09 to de6570f Compare May 5, 2023 20:14

aarzilli mentioned this pull request May 16, 2023

cmd/link: there should be a way to determine why deadcode elimination was not performed golang/go#60221

Closed

aarzilli force-pushed the dereflect branch from de6570f to 8159848 Compare February 10, 2024 13:43

aarzilli force-pushed the dereflect branch from 8159848 to a9601b1 Compare April 10, 2024 08:00

aarzilli force-pushed the dereflect branch from a9601b1 to b06beaa Compare May 10, 2024 14:50

aarzilli force-pushed the dereflect branch from b06beaa to e94761a Compare June 13, 2024 10:00

aarzilli force-pushed the dereflect branch from e94761a to ee7aba8 Compare August 10, 2024 11:31

marckhouzam changed the title ~~Restructure code to let linker perform deadcode elimination step~~ Allow linker to perform deadcode elimination for program using Cobra Oct 14, 2024

marckhouzam reviewed Oct 15, 2024

View reviewed changes

cobra_test.go Outdated Show resolved Hide resolved

cobra_test.go Outdated Show resolved Hide resolved

cobra_test.go Outdated Show resolved Hide resolved

cobra_test.go Show resolved Hide resolved

aarzilli force-pushed the dereflect branch from ee7aba8 to 4da4d97 Compare October 15, 2024 08:19

marckhouzam merged commit 611e16c into spf13:main Jan 27, 2025
20 checks passed

marckhouzam mentioned this pull request Feb 2, 2025

Allow to reset the templates to the default #2229

Merged

sylr mentioned this pull request Feb 13, 2025

Add a noexprlang build tag nats-io/jsm.go#629

Open

pgimalac mentioned this pull request Feb 17, 2025

Update cobra to v1.9.1 DataDog/datadog-agent#34081

Merged

This was referenced Feb 17, 2025

*: remove uses of reflect.MethodByName from all of Delve go-delve/delve#3916

Merged

logflags: replace logrus with log/slog go-delve/delve#3918

Merged

marckhouzam mentioned this pull request Feb 18, 2025

Don't use templating to allow for dead-code elimination vmware-tanzu/tanzu-plugin-runtime#228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow linker to perform deadcode elimination for program using Cobra #1956

Allow linker to perform deadcode elimination for program using Cobra #1956

aarzilli commented May 5, 2023 •

edited by marckhouzam

Loading

CLAassistant commented May 5, 2023 •

edited

Loading

github-actions bot commented May 5, 2023

aarzilli commented May 5, 2023

github-actions bot commented May 5, 2023

marckhouzam commented Jun 12, 2023

aarzilli commented Jun 12, 2023

aarzilli commented Jul 28, 2023

aarzilli commented Oct 10, 2023

github-actions bot commented Feb 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented May 10, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Aug 10, 2024

marckhouzam commented Oct 14, 2024 •

edited

Loading

aarzilli commented Oct 14, 2024 •

edited

Loading

marckhouzam commented Oct 15, 2024

marckhouzam left a comment

marckhouzam commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Jan 27, 2025

artem-anisimov-0x7f commented Jan 27, 2025

marckhouzam commented Jan 27, 2025

marckhouzam commented Jan 27, 2025

aarzilli commented Jan 27, 2025

sylr commented Jan 29, 2025

artem-anisimov-0x7f commented Feb 12, 2025

marckhouzam commented Feb 15, 2025

Allow linker to perform deadcode elimination for program using Cobra #1956

Allow linker to perform deadcode elimination for program using Cobra #1956

Conversation

aarzilli commented May 5, 2023 • edited by marckhouzam Loading

CLAassistant commented May 5, 2023 • edited Loading

github-actions bot commented May 5, 2023

aarzilli commented May 5, 2023

github-actions bot commented May 5, 2023

marckhouzam commented Jun 12, 2023

aarzilli commented Jun 12, 2023

aarzilli commented Jul 28, 2023

aarzilli commented Oct 10, 2023

github-actions bot commented Feb 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented May 10, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Aug 10, 2024

marckhouzam commented Oct 14, 2024 • edited Loading

aarzilli commented Oct 14, 2024 • edited Loading

marckhouzam commented Oct 15, 2024

marckhouzam left a comment

Choose a reason for hiding this comment

marckhouzam commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

aarzilli commented Jan 27, 2025

artem-anisimov-0x7f commented Jan 27, 2025

marckhouzam commented Jan 27, 2025

marckhouzam commented Jan 27, 2025

aarzilli commented Jan 27, 2025

sylr commented Jan 29, 2025

artem-anisimov-0x7f commented Feb 12, 2025

marckhouzam commented Feb 15, 2025

aarzilli commented May 5, 2023 •

edited by marckhouzam

Loading

CLAassistant commented May 5, 2023 •

edited

Loading

marckhouzam commented Oct 14, 2024 •

edited

Loading

aarzilli commented Oct 14, 2024 •

edited

Loading